Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normalize the package hash to hex. #512

Merged
merged 1 commit into from
Nov 23, 2021

Conversation

mattmoor
Copy link
Collaborator

We were emitting package checksum hashes as h1:{base64}. h1: is a prefix that indicates "Hash 1", which is a SHA-256 based hash of the files, which is then base64 encoded as the suffix.

This change detects/strips the h1: prefix and re-encodes the base64 data as hex.

I also combined the two loops over .Deps since I noticed bom doing things in this order. 🤷

@mattmoor mattmoor requested a review from imjasonh November 22, 2021 23:23
@google-cla google-cla bot added the cla: yes label Nov 22, 2021
@mattmoor mattmoor force-pushed the convert-hash1-to-sha256 branch from 1d73d0a to 90dd316 Compare November 22, 2021 23:31
@mattmoor
Copy link
Collaborator Author

So it looks like for whatever reason locally I get:

dep	github.com/google/go-containerregistry	v0.7.0	h1:u0onUUOcyoCDHEiJoyR1R1gx5er1+r06V5DBhUU5ndk=

However, in CI we're seeing:

dep	github.com/google/go-containerregistry	v0.7.0	

Not sure what's going on here, but seems like the Sum field should be considered optional in our SPDX conversion.

cc @imjasonh

@mattmoor mattmoor force-pushed the convert-hash1-to-sha256 branch from 1ef43c9 to 90dd316 Compare November 22, 2021 23:50
We were emitting package checksum hashes as `h1:{base64}`.  `h1:` is a prefix that indicates "Hash 1", which is a SHA-256 based hash of the files, which is then base64 encoded as the suffix.

This change detects/strips the `h1:` prefix and re-encodes the base64 data as hex.
@mattmoor mattmoor force-pushed the convert-hash1-to-sha256 branch from 90dd316 to 149252f Compare November 22, 2021 23:54
@mattmoor
Copy link
Collaborator Author

For now I've predicated the PackageChecksum on .Sum being present, but we should figure out if there's a way to ensure this comes out.

@codecov-commenter
Copy link

Codecov Report

Merging #512 (149252f) into main (3edb68b) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #512      +/-   ##
==========================================
+ Coverage   50.51%   50.60%   +0.09%     
==========================================
  Files          41       41              
  Lines        2051     2051              
==========================================
+ Hits         1036     1038       +2     
+ Misses        840      838       -2     
  Partials      175      175              
Impacted Files Coverage Δ
pkg/commands/resolver.go 31.57% <100.00%> (+0.95%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3edb68b...149252f. Read the comment docs.

Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, but I agree the missing sum is confusing. Can we have kind-e2e.yaml also run go version -m directly to help debug?

@imjasonh imjasonh merged commit 5787600 into ko-build:main Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants